Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CMake: Workaround clang17+ / lto bug #585

Merged
merged 1 commit into from
Sep 27, 2024
Merged

Conversation

jschueller
Copy link
Contributor

No description provided.

@jschueller jschueller force-pushed the rtti branch 2 times, most recently from 76ca64a to 1ce48f2 Compare September 26, 2024 09:10
@bluescarni
Copy link
Member

bluescarni commented Sep 26, 2024

@jschueller thanks for the fast PR!

As far as I understand it, for the LLVM bug to trigger, link-time optimisation must be enabled - at least judging from this bugreport:

llvm/llvm-project#71196

I was thus wondering if it would make sense to just disable LTO if clang>=17 is detected, rather than injecting the -fno-assume-unique-vtables flag. I am not sure though, what do you think?

EDIT: I am talking about the PAGMO_ENABLE_IPO option, just to be clear.

@jschueller
Copy link
Contributor Author

jschueller commented Sep 26, 2024

I propose to keep the new flag, but only use it when IPO is enabled
your solution would be way shorter though, you decide

@jschueller jschueller force-pushed the rtti branch 2 times, most recently from d9d745a to 2cdd13c Compare September 26, 2024 09:35
@jschueller jschueller marked this pull request as ready for review September 26, 2024 09:37
@jschueller jschueller changed the title rtti? CMake: Workround clang17+ / lto bug Sep 26, 2024
@jschueller
Copy link
Contributor Author

will close #579

@bluescarni
Copy link
Member

@jschueller sorry for getting back to this only now...

As far as I understand from #586, switching off IPO actually did not prevent the bug from manifesting right? If that is the case and it is ok with you, I would slightly edit this PR to enable -fno-assume-unique-vtables regardless of whether or not IPO is enabled.

@jschueller
Copy link
Contributor Author

jschueller commented Sep 27, 2024

oh, right, I did not take that into account
done

@jschueller jschueller changed the title CMake: Workround clang17+ / lto bug CMake: Workaround clang17+ / lto bug Sep 27, 2024
@bluescarni
Copy link
Member

@jschueller thank you!

@bluescarni bluescarni merged commit 5e22ae1 into esa:master Sep 27, 2024
8 checks passed
@jschueller jschueller deleted the rtti branch September 27, 2024 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants